support to delete pipelinerun#1069
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the capability to delete PipelineRun resources, providing a new API endpoint for this operation. It includes robust logic to prevent accidental deletion of active pipeline runs and offers flexibility to retain associated Jenkins build records if desired. The changes ensure proper resource lifecycle management and cleanup within the system. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds support for deleting PipelineRun resources. The implementation includes a new API endpoint for deletion, with an option to keep the corresponding Jenkins job history. The controller logic is updated with a finalizer to handle the cleanup of Jenkins resources upon PipelineRun deletion. The changes are well-structured and the logic is sound. I have a couple of suggestions to improve the robustness of the controller's update operations.
| pipelineRunCopied.ObjectMeta.Finalizers = sliceutil.RemoveString(pipelineRunCopied.ObjectMeta.Finalizers, func(item string) bool { | ||
| return item == v1alpha3.PipelineRunFinalizerName | ||
| }) | ||
| err = r.Update(context.TODO(), pipelineRunCopied) |
There was a problem hiding this comment.
This update to remove the finalizer should be wrapped in a retry.RetryOnConflict loop to handle potential conflicts gracefully. This is a standard pattern in controllers to avoid reconciliation failures due to object modifications between read and write operations. You can see an example of this pattern in the updateStatus function in this file.
Additionally, please use the ctx from the Reconcile function instead of context.TODO().
| // add finalizer if it does not exist | ||
| if !sliceutil.HasString(pipelineRunCopied.ObjectMeta.Finalizers, v1alpha3.PipelineRunFinalizerName) { | ||
| pipelineRunCopied.ObjectMeta.Finalizers = append(pipelineRunCopied.ObjectMeta.Finalizers, v1alpha3.PipelineRunFinalizerName) | ||
| if err := r.Update(ctx, pipelineRunCopied); err != nil { |
There was a problem hiding this comment.
This update to add the finalizer should be wrapped in a retry.RetryOnConflict loop to handle potential conflicts gracefully. This is a standard pattern in controllers to avoid reconciliation failures due to object modifications between read and write operations. You can see an example of this pattern in the updateStatus function in this file.
| } else { | ||
| k8sutil.RemoveFinalizer(&pipelineRunCopied.ObjectMeta, v1alpha3.PipelineRunFinalizerName) | ||
| err = r.Update(context.TODO(), pipelineRunCopied) | ||
| return ctrl.Result{}, err |
There was a problem hiding this comment.
i'm afraid this error will occur continuously in some cases, for example, the jenkins job has already been deleted, or the jenkins job can not be deleted due to some reasons (running, network issues etc).
suggest just log the error and don't return, continue the below code
| if !pipelineRunCopied.ObjectMeta.DeletionTimestamp.IsZero() { | ||
| if err = jHandler.deleteJenkinsJobHistory(pipelineRunCopied); err != nil { | ||
| // if the annotation value is true, we should keep the record in Jenkins | ||
| if keep, err := strconv.ParseBool(pipelineRunCopied.Annotations[v1alpha3.PipelineRunKeepJenkinsRecordAnnoKey]); err == nil && keep { |
There was a problem hiding this comment.
pipelineRunCopied.Annotations may be nil map.
There was a problem hiding this comment.
We do not perform write operations on the map here. Reading a nil map will return its default empty value, and no panic will occur.
Signed-off-by: renyunkang <rykren1998@gmail.com>
|
/lgtm |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes # https://github.com/kubesphere/project/issues/7185
Special notes for reviewers:
Please check the following list before waiting reviewers:
Does this PR introduce a user-facing change??